Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Resolve 2 spotbugs issues and increase spotbugs checks #100

Merged
merged 12 commits into from
Feb 11, 2023
Merged

Resolve 2 spotbugs issues and increase spotbugs checks #100

merged 12 commits into from
Feb 11, 2023

Conversation

prince-panwar
Copy link
Contributor

  • Make sure you are opening from a topic/feature/bugfix branch (right side) and not your main branch!
  • Ensure that the pull request title represents the desired changelog entry
  • Please describe what you did

prince-panwar and others added 3 commits February 9, 2023 17:24
@prince-panwar
Deeper spotbugs checks
520c069 
Increase chance of bug detecting
@prince-panwar @MarkEWaite
Update pom.xml
dfe0019 
Co-authored-by: Mark Waite <mark.earl.waite@gmail.com>
@prince-panwar
resolved 3 bugs spotted by spotbugs
030b389
@prince-panwar prince-panwar requested a review from a team as a code owner February 10, 2023 16:05
@prince-panwar
Copy link
Contributor Author

hi, mark I have successfully fixed 3 out of 5 bugs , but I am stuck at two similar bugs:-
In the code, both the gitLabAPI and me fields are marked as transient, which means they will not be persisted when the object is serialized. This is leading to the error "SE_TRANSIENT_FIELD_NOT_RESTORED" indicates that a field marked as transient has not been restored properly after being deserialized.
can you help me out here?

@prince-panwar
Copy link
Contributor Author

I am not sure what is the best way to deal with these bugs

@MarkEWaite
Copy link
Contributor

can you help me out here?

Add an exclusions filter file as described in the tutorial. Exclude those two issues.

It is also OK to include all 5 issues in the exclusions filter file. Excluding a spotbugs warning does not make the code any worse and reduces the risk that future changes will introduce issues that could have been detected by spotbugs.

MarkEWaite
MarkEWaite requested changes Feb 10, 2023
View reviewed changes
Copy link
Contributor

@MarkEWaite MarkEWaite left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! I think that some of the changes may be safer to exclude in the filter file rather than make a change in the production code. Alternate ideas are welcome. See my questions with individual changes.

src/main/java/org/jenkinsci/plugins/GitLabAuthenticationToken.java Outdated Show resolved Hide resolved
src/main/java/org/jenkinsci/plugins/GitLabOAuthUserDetails.java Show resolved Hide resolved
src/main/java/org/jenkinsci/plugins/GitLabSecurityRealm.java Outdated Show resolved Hide resolved
src/main/java/org/jenkinsci/plugins/GitLabSecurityRealm.java Outdated Show resolved Hide resolved
@prince-panwar
Copy link
Contributor Author

thanks, I'll fix these in next commit

@prince-panwar
spotbugs exclusin file added
aa0e0c6 
to suppress some bugs that are safe to be ignored
@prince-panwar
Copy link
Contributor Author

I added the exclusion file but spot bugs is still showing bugs . what am i doing wrong?

@MarkEWaite
Copy link
Contributor

MarkEWaite commented Feb 10, 2023 •
edited
Loading

I added the exclusion file but spot bugs is still showing bugs . what am i doing wrong?

There were several mistakes in src/spotbugs/excludesFilter.xml. I've updated the file to correct those mistakes in e42bc19

Mistakes included:

  • Used Class when you also needed to use Field. I switched it to use both Class and Field
  • Used Class when you also needed to use Method. I switched it to use both Class and Method
  • Used more complicated And and Or when declaring multiple Match conditions is easier to read. I switched to multiple top level Match conditions

Thanks for the submission!

@MarkEWaite
Copy link
Contributor

I also set the checkstyle version in 0b32055 in order to prevent a maven warning at compile time. I'm not sure why that wasn't detected previously, but it is resolved now.

@MarkEWaite
Copy link
Contributor

I needed b7510e7 to undo the change to the setValue method otherwise it would have entered the switch with a lowercase value and then been checking strings that contain uppercase letters. That would have failed.

@MarkEWaite MarkEWaite mentioned this pull request Feb 10, 2023
Closed
3 tasks
@MarkEWaite MarkEWaite changed the title 3 spotbugs issue resolved Resolve 2 spotbugs issues and increase spotbugs checks Feb 10, 2023
@MarkEWaite MarkEWaite self-requested a review February 10, 2023 21:03
MarkEWaite
MarkEWaite approved these changes Feb 10, 2023
View reviewed changes
Copy link
Contributor

@MarkEWaite MarkEWaite left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approved with changes as they are now.

@MarkEWaite
Copy link
Contributor

Should be squash merged when it is merged.

@prince-panwar
Replaced hard-coded value for user constructor
b2b71a2 
with email property from gitlab4j.api.models.User

hard coded values might not be appropriate for all cases

may cause issues in the application
MarkEWaite
MarkEWaite requested changes Feb 11, 2023
View reviewed changes
Copy link
Contributor

@MarkEWaite MarkEWaite left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The new call to user.getEmail() needs to be removed. It does not belong in this pull request.

MarkEWaite
MarkEWaite approved these changes Feb 11, 2023
View reviewed changes
Copy link
Contributor

@MarkEWaite MarkEWaite left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks again!

@MarkEWaite MarkEWaite merged commit e49bae6 into jenkinsci:master Feb 11, 2023
@prince-panwar prince-panwar deleted the 3-spotbugs-issue-resolved branch February 11, 2023 18:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@MarkEWaite MarkEWaite MarkEWaite approved these changes

Assignees
No one assigned
Labels
chore
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@prince-panwar @MarkEWaite